Fixing Github contribution error #650
Conversation
Yemaneberhan-Lemma
temporarily deployed
to
feature
GitHub Actions
Inactive
Yemaneberhan-Lemma
temporarily deployed
to
feature
GitHub Actions
Inactive
Yemaneberhan-Lemma
temporarily deployed
to
feature
GitHub Actions
Inactive
There was a problem hiding this comment.
While the backoff algorithm is good, you are doing a lot here for a simple fix. Because this is likely to be stuck in review, I'd encourage you to create another PR with the minimal fix so that we can merge it quickly. After that, we can talk about other improvements, including the back-off algorithm in this PR.
| $config = $config_factory->get('ct_github.settings'); | ||
| $token = $config->get('github_auth_token'); | ||
| $client = new Client(); | ||
| $client->authenticate($token, NULL, AuthMethod::ACCESS_TOKEN); | ||
| $client = (strlen($token) === 40) ? (new Client())->authenticate($token, NULL, AuthMethod::ACCESS_TOKEN) : NULL; |
There was a problem hiding this comment.
Not all tokens are 40 characters in length. Fine-grained tokens are longer.
| @@ -40,13 +41,14 @@ class GithubQuery { | |||
| * @param \Drupal\Core\Cache\CacheBackendInterface $cacheBackend | |||
| * The injected cache backend service. | |||
| */ | |||
| public function __construct(ConfigFactory $config_factory, CacheBackendInterface $cacheBackend) { | |||
| public function __construct(ConfigFactory $config_factory, CacheBackendInterface $cacheBackend, LoggerChannelFactoryInterface $loggerFactory) { | |||
There was a problem hiding this comment.
Extra whitespace. This should have been caught by the CI. This is another issue but let's not introduce this here.
| if ($e->getMessage() === 'Bad credentials') { | ||
| $this->logger->error('GitHub API error: Bad credentials. Please check the credentials.'); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Is it likely that we will get bad credentials here? Shouldn't this have been caught in authenticate method itself?
|
|
||
| // Handle other errors with backoff | ||
| $retryCount++; | ||
| $this->logger->warning("GitHub API request failed. Retry $retryCount/$maxRetries. Error: " . $e->getMessage()); |
There was a problem hiding this comment.
Let's not log this at warning when we are going to try again. We can log this at info or notice level maybe.
| if($userContributions){ | ||
| $issues = array_map(function ($issue) { | ||
| return new Issue($issue['title'], $issue['url']); | ||
| }, $userContributions['data']['user']['issues']['nodes']); | ||
| return $issues; | ||
| return $issues;} |
There was a problem hiding this comment.
Whitespace error. Again this should have been caught in the CI.
| if ($plugin_id === "github") { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is a code smell. The ct_manager should not have to know about ct_github.
|
@Yemaneberhan-Lemma, this needs to be rebased. |
Handling the issue